-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass any variables between stages #45
base: master
Are you sure you want to change the base?
Conversation
* | ||
* @param callable $stage | ||
* @return \League\Pipeline\PipelineInterface|$this | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, PipelineInterface
already provides all the docblocks. Same for other docblocks added in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For others -- ok. But for this docblock for PipelineInterface
returns only interface without $this
. IDE autocompletion works only within interface scope and not touch process
method which should be there.
{ | ||
use ParametersTrait; | ||
|
||
public function process($payload, callable ...$stages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I forgot about this signature, makes it significantly trickier to handle. It's pretty straight forward for the Pipeline:
function process(...$parameters) { ... }
But the processor is different because it receives the stages:
function process($payload, callable ...$stages) { ... }
$this->processor->setParameters(...$params); | ||
return $this->processor->process($payload, ...$this->stages); | ||
} | ||
|
||
return $this->processor->process($payload, ...$this->stages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense for this to be:
return $this->processor->withParameters($params)->process(...$this->stages);
Doing so will change the signatures significantly though, and cause a major version bump.
Let me think about this for a couple of days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, your variant is more elegant, but needs to bump the major version.
Let me think about this for a couple of days.
ok.
Work example:
Also I added docblocks for better IDE autocomplete. Full backward compatibility.